-
-
Notifications
You must be signed in to change notification settings - Fork 132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement wait_for_selector #236
base: main
Are you sure you want to change the base?
Conversation
05e5d00
to
bc13315
Compare
bc13315
to
83ca882
Compare
f435ba9
to
3b09739
Compare
lib/ferrum/frame/dom.rb
Outdated
@@ -36,6 +36,33 @@ def body | |||
evaluate("document.documentElement.outerHTML") | |||
end | |||
|
|||
def wait_for_selector(css: nil, xpath: nil, timeout: 3000, interval: 100) | |||
evaluate_func(%( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes sense, will change, thanks!
lib/ferrum/frame/dom.rb
Outdated
waitForSelector(resolve, reject); | ||
}); | ||
} | ||
), css || xpath, css.nil? && !xpath.nil?, timeout, interval, awaitPromise: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was feeling like this arguments list was a little crowded, and also noticed this function could be incorrectly called without raising an exception: one of css
or xpath
is required, but not handled. What do you think about something like this:
def wait_for_selector(css: nil, xpath: nil, timeout: 3000, interval: 100)
raise ArgumentError.new("Provide :css or :xpath, but not both") if css && xpath
raise ArgumentError.new("Provide :css or :xpath") unless css || xpath
evaluate_func(<<~JS, css || xpath, !!xpath, timeout, interval, awaitPromise: true)
function(selector, isXpath, timeout, interval) {
var attempts = 0;
The guard clauses protect the arguments from being called incorrectly, and also simplify the resolution to isXpath
, since it can now only be one or the other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, it looks like too much for one method to be responsible for selector/XPath and also for validations.
However, I see the confusing point here as well. In this case, I think we should use simple methods for more clear API, like those:
def wait_for_selector(css, **options)
wait_for_element(css: css, **options)
end
def wait_for_xpath(xpath, **options)
wait_for_element(xpath: xpath, **options)
end
The point to merge two ways of element-find logic was a follow a DRY principle for a common JS function, especially this part:
var element = isXpath
? document.evaluate(selector, document, null, XPathResult.FIRST_ORDERED_NODE_TYPE, null).singleNodeValue
: document.querySelector(selector);
therefore, I think, we can find a way to refactor it with a private function by passing a js function, gimme see closer, will do an update soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, yes, I think this is a better direction. Consider wait_for_css
rather than wait_for_selector
as a selector could generically mean css or xpath. It makes the expected arg more clear.
Can this be merged? Do the linters need to be changed or do we want to follow the linter advice? |
I'm sorry guys, with all this stuff happening I barely find time for open source now, hope it's going to calm down soon. |
If anyone needs a simple substitute of timeout_true = lambda do |timeout = 1, &block|
Timeout.timeout timeout do
block.yield or (sleep 0.1; redo)
end
end |
Any chance we could have a reviewer review and accept this? It would be very nice to have in the main Gem. Thanks! |
Would this be simpler? class Ferrum::Browser
def wait_for(want, wait:1, step:0.1)
meth = want.start_with?('/') ? :at_xpath : :at_css
until node = send(meth, want)
(wait -= step) > 0 ? sleep(step) : break
end
node
end
end |
closes #82